-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Add] IEquatable
to Signer
#3571
base: master
Are you sure you want to change the base?
[Add] IEquatable
to Signer
#3571
Conversation
|
||
public override bool Equals(object obj) | ||
{ | ||
if (obj == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
== null already done in is sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==
checks for null
and sends to this method.
if (other is null) return false; | ||
return Account == other.Account && | ||
Scopes == other.Scopes && | ||
AllowedContracts.SequenceEqual(other.AllowedContracts) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these values are null depending of the Scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope can not be null, its a enum, can not be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope can not be null,
But AllowedContracts
/AllowedGroups
/Rules
can be null sometimes, that's what Shargon says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, some of the values can be null, we should make unit tests with real signers with different scopes, depending of the scope, you will find nulls entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Xxxs.AsSpan().SequenceEqual(other.AsSpan())
instead.
SequenceEqual
in System.MemoryExtension
s is null safe.
if (other is null) return false; | ||
return Account == other.Account && | ||
Scopes == other.Scopes && | ||
AllowedContracts.SequenceEqual(other.AllowedContracts) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope can not be null,
But AllowedContracts
/AllowedGroups
/Rules
can be null sometimes, that's what Shargon says.
@@ -66,6 +67,31 @@ public class Signer : IInteroperable, ISerializable | |||
/*AllowedGroups*/ (Scopes.HasFlag(WitnessScope.CustomGroups) ? AllowedGroups.GetVarSize() : 0) + | |||
/*Rules*/ (Scopes.HasFlag(WitnessScope.WitnessRules) ? Rules.GetVarSize() : 0); | |||
|
|||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
public bool Equals(Signer other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a practical need for this API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those of being nulls will also be compared, i have checked, null == null return true. Not sure if this can be of any problem, looks right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not bad to me also, == and != not required to me, but it's ok
Change Log
IEquatable
toSigner
classIEquatable
onSigner
classNeeds #3573
Type of change
How Has This Been Tested?
Test_IEquatable
Test Configuration:
Checklist: